- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6.4k
feat(search): use new Orama components #8175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
| 
 🙂 Sure, I'll manage this today @avivkeller. Before I update the workflow to include them, could you confirm whether the variables below are already set as secrets? And if there's any distinction between those used in prod and preview environments that I should take into account? Would the following update to the test step look correct to you? Thank you! | 
| @avivkeller would the GHA snippet above be OK? | 
| (Forget my deleted comment above) No, the snippet does not look good to me. Playwright should run without secrets. We could just remove the Orama tests, and accept that this is a third-party provider not subject to our tests. | 
| The "Sync Orama Cloud" test is an expected failure given the changes, but can someone confirm that a local call of that script works with the secrets defined? If not, I can make a temporary branch to test it. | 
| 
 Makes sense for me, I can remove the Orama tests | 
| const hasInteractions = !!interactions?.length; | ||
|  | ||
| useEffect(() => { | ||
| setTimeout(() => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a cleanup function of the effect to cancel the setTimeout during component unmount.
| const displaySearch = | ||
| !isMobileScreen || (isMobileScreen && searchbox.mode === 'search'); | ||
|  | ||
| useEffect(() => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please use our existing useMediaQuery hook here? https://github.com/nodejs/nodejs.org/blob/main/apps/site/hooks/react-client/useMediaQuery.ts instead of this isMobileScreen state
| import { Search } from './Search'; | ||
| import { SlidingChatPanel } from './SlidingChatPanel'; | ||
|  | ||
| const InnerSearchboxModal: FC<PropsWithChildren<{ onClose: () => void }>> = ({ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component should reside in another file. Policy here is one component per file :)
| const orama = useOrama(); | ||
| const t = useTranslations(); | ||
|  | ||
| useEffect(() => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of reusability and maintainability could you make a dedicated hook for this? Or use an existing Radix component that could do this? I feel I've mentioned this before?
| apiKey: ORAMA_CLOUD_READ_API_KEY, | ||
| }); | ||
| } | ||
| return null; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to return null. Make the context be OramaCloud | undefined, createContext by default supports "undefiend", so you only need one return statement.
|  | ||
| export const OramaProvider = ({ children }: { children: React.ReactNode }) => { | ||
| const instance = useMemo(() => { | ||
| if (ORAMA_CLOUD_PROJECT_ID && ORAMA_CLOUD_READ_API_KEY) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these constants are defined statically and won't change during runtime, wouldn't it make sense to create the instance outside of the provider? Does it need to be a react provider? Is it because we want to ensure this is client-side only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add 'use client';, React Providers are client-side only and we rather be explicit when something is client-side only.
|  | ||
| export const useSearchbox = (): SearchboxContextType => { | ||
| const context = useContext(SearchboxContext); | ||
| if (!context) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to throw an Error explicitly. It will throw an error for whoever is consuming this anyways.
| const searchDispatch = useSearchDispatch(); | ||
|  | ||
| const clearAll = useCallback(() => { | ||
| searchDispatch({ type: 'SET_SEARCH_TERM', payload: { searchTerm: '' } }); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you make this an entry on your reducer named CLEAR_ALL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're almost there. I've left a final set of comments. After this we're good to go!
| 
 Hey @ovflowd, thank you for this last review! I'll try to manage everything by tomorrow. I'll keep you posted. | 
This PR supersedes #7971 and integrates the new Orama components, powered by the new OramaCore backend.
Description
New components - React-based to replace the old WebComponents-based ones. New backend (OramaCore instead of old Orama Cloud), all hosted and maintained on https://app.orama.com. Credentials have been shared privately with the repository maintainers on Slack.
Validation
Tested locally and on remote demo environment.
Related Issues
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.